@dustinswales I am happy to approve this PR now, but I am wondering if it's still needed given #651?

@DomHeinzeller After discussion in #651, the preferable solution is to redo this one debug check to check the length of each dimension, and not use the bounds of the array.
Then I could remove this new attribute.

This will take some work, and I will not be able to get to this for a few days.

Ok, thanks. Please ping me when the revised version of this PR is ready.

When these changes are done, I think an adjustment of the Input/Output Variable (argument) Rules is in order.

One idea is to modify the warning and example to:

.. warning:: Fortran assumes that the lower bound of assumed-size arrays is ``1``. If a scheme uses ``foo`` with lower bounds ``is`` and ``ks`` that are different from ``1``, then these must be specified explicitly:

  .. code-block:: fortran

     real(kind=kind_phys), dimension(is:,ks:), intent(inout) :: foo

@gold2718 I've made the discussed modifications. This is ready for review @climbfuji @peverwhee

@gold2718 I can't figure out how to trigger these errors!?!

I haven't really thought hard about this, but do we need these size checks at all?
If host and scheme files have differing dimensions, it will get caught during parsing, so isn't this check redundant? (i.e. there isn't anything going on in the Cap that wasn't described in the metadata files)
@climbfuji @peverwhee

@gold2718 I can't figure out how to trigger these errors!?!

I haven't really thought hard about this, but do we need these size checks at all? If host and scheme files have differing dimensions, it will get caught during parsing, so isn't this check redundant? (i.e. there isn't anything going on in the Cap that wasn't described in the metadata files) @climbfuji @peverwhee

The purpose of this is not to check if the metadata is consistent between host and scheme. It's to check if the allocation of the variable by the host is different than what the metadata says.

@gold2718 I can't figure out how to trigger these errors!?!

I haven't really thought hard about this, but do we need these size checks at all? If host and scheme files have differing dimensions, it will get caught during parsing, so isn't this check redundant? (i.e. there isn't anything going on in the Cap that wasn't described in the metadata files) @climbfuji @peverwhee

capgen is thwarting us!

See how var_array is being passed into the suite cap from test_host_ccpp_cap.F90:

var_array=var_array(col_start:col_end, 1:2, 1:4, 1:6))

@gold2718 @DomHeinzeller @dustinswales I have opened a PR for changing the tech doc with the suggested wording: NCAR/ccpp-doc#75

I am not so sure about the intents, though. But that might be because I am still confused about the intent. An intent(out) doesn't mean the variable must be allocated by the scheme, or? It could just mean that the scheme updates the information (completely) for an already allocated variable.

This is not correct. Upon subroutine entry, an intent(out) allocatable array becomes "not currently allocated" (and thus, is deallocated if necessary). This is a necessary feature of the language because it is not allowed to allocate an already allocated (allocatable) array (try it).

@gold2718 I am referring to this situation of which there are probably more than 100 in the current ccpp-physics: (bar takes the place of the scheme _run, _timestep_init, etc subroutine):

program alloctest

  implicit none

  real, allocatable :: foo(:)

  allocate(foo(1:7))
  foo = 0.0
  call bar(foo)
  
  print *, shape(foo)
  print *, foo

contains

  subroutine bar(foo)
    real, intent(out) :: foo(:)
    foo = 7.0
  end subroutine bar

end program alloctest

with result

[dom@bounty alloctest]$ gfortran -o main.x main.F90
[dom@bounty alloctest]$ ./main.x
           7
   7.00000000       7.00000000       7.00000000       7.00000000       7.00000000       7.00000000       7.00000000

This is how the intent is used in the ccpp-physics and ccpp-framework (prebuild) as of today. If something is intent(out), then it means the data must be overwritten entirely, it does not imply or even allow a reallocation of the array. Remember that in the good old days, the rule was that the host model is responsible for ALL memory management.

This is how the intent is used in the ccpp-physics and ccpp-framework (prebuild) as of today. If something is intent(out), then it means the data must be overwritten entirely, it does not imply or even allow a reallocation of the array. Remember that in the good old days, the rule was that the host model is responsible for ALL memory management.

@climbfuji This was my understanding.
Capgen adds a wrinkle to this by "offloading the memory mgmt" by creating suite cap variables that aren't defined/allocated by the host. However, that doesn't mean the host won't still manage all the memory themselves, which is the status of the UFS physics.

This is how the intent is used in the ccpp-physics and ccpp-framework (prebuild) as of today.

Thanks for the example, I now see we are talking about different things. You are talking about a regular, intent(out) array while I was talking about "internally-generated scheme allocatable arrays". The key difference is that for my examples, bar would look like:

  subroutine bar(foo)
    real, allocatable, intent(out) :: foo(:)
    foo = 7.0
  end subroutine bar

which would not work!

If something is intent(out), then it means the data must be overwritten entirely, it does not imply or even allow a reallocation of the array.

How would you test that? There is nothing in Fortran (aside from possible compiler warnings) that enforces that. In your example, if you change the assignment to foo(3) = 7.0, then even --warn-all on gfortran will not complain.

This is not about testing whether the data must be overwritten entirely. This is about having to test the allocation of the array if allocated by the host model, regardless of whether the intent is in, inout, or out (in reply to comment #652 (comment)).

And also, you can catch the missing assignment of data to an intent(out) array quite easily with generated code. For example, when debug checks are enabled, every array that is declared as intent(out) can be assigned a value that would either cause a runtime error if being used in the scheme, or that can be tested for after the scheme was called.

This is about having to test the allocation of the array if allocated by the host model, regardless of whether the intent is in, inout, or out (in reply to comment #652 (comment)).

I guess that is why I am confused. I did call for testing all "host model" allocatable arrays or pointers in the host cap. The only other arrays which need to be tested are intent(out) scheme allocatable arrays and these would be tested in the suite cap. It sounds like the these scheme tests would never be generated for UFS schemes but other schemes could have such arrays. I do not think we have a disagreement here.

And also, you can catch the missing assignment of data to an intent(out) array quite easily with generated code. For example, when debug checks are enabled, every array that is declared as intent(out) can be assigned a value that would either cause a runtime error if being used in the scheme, or that can be tested for after the scheme was called.

This is only the case if you reserve one or more values to the CCPP Framework. Otherwise, a scheme could use any value (including huge(1.0_kind_phys)) for some internal purpose. If you did make such a reservation, you could set it on the way in and test that no such values are in the array on the way out.

One candidate that might be acceptable is NaN. Some compilers have a flag to initialize all data to NaN so that tests can be run to catch the use of uninitialized data.

And also, you can catch the missing assignment of data to an intent(out) array quite easily with generated code. For example, when debug checks are enabled, every array that is declared as intent(out) can be assigned a value that would either cause a runtime error if being used in the scheme, or that can be tested for after the scheme was called.

This is only the case if you reserve one or more values to the CCPP Framework. Otherwise, a scheme could use any value (including huge(1.0_kind_phys)) for some internal purpose. If you did make such a reservation, you could set it on the way in and test that no such values are in the array on the way out.

One candidate that might be acceptable is NaN. Some compilers have a flag to initialize all data to NaN so that tests can be run to catch the use of uninitialized data.

I was thinking the same, yes!

With #658, I think we should go ahead and merge this PR. Can you take another look, please, @gold2718?

Also, is there a reason you are testing size(var_array(1,1,:,1)) instead of size(var_array, 3)?

@gold2718 Not that I recall.

@gold2718 @climbfuji @peverwhee
I made two changes that need re-review before merging.

1. Fix error messaging issue in Debug checks output unreadable writes #590 (see autogenerated code snippet below).
2. Skip "length check" for 1D arrays, since same as "size check" in this case (see "ps" below)

@gold2718 @climbfuji @peverwhee I made two changes that need re-review before merging.

1. Fix error messaging issue in Debug checks output unreadable writes #590 (see autogenerated code snippet below).
2. Skip "length check" for 1D arrays, since same as "size check" in this case (see "ps" below)

Thanks @dustinswales, this all looks good to me. Happy to proceed with merging the PR as it is now.

@dustinswales looks good to me!